fix(security): shared lessons print inert in the CLI; widths count wide glyphs - #153

Point 2 of the UI/UX review. Supersedes #134, closed on 1 Nov to go through the plan one point at a time. Same change (sanitising, widths and colour together, as approved), rebased on develop.

This is a security fix. On develop (and every release up to 2.7.1), the CLI writes text from a shared lesson to the terminal byte for byte. A shared lesson is someone else's text. Checked on a sandbox: after devcoach import of a crafted share code,

• the title Cache mounts survive rebuilds ESC[2J ESC[3J ESC[H clears the screen and the scrollback when import, lessons or lesson <id> prints it;
• a sender name ESC]8;;javascript:alert(1) BEL Ada ESC]8;; BEL and a summary link to file:///etc/passwd print as live OSC 8 hyperlinks behind innocent text;
• an OSC 52 sequence in the summary writes the clipboard on terminals that honour it, when lesson <id> prints it.

The bytes were also stored as-is in coaching.db, so every surface received them. The dashboard escapes HTML, so this is the terminal surface.

Scope: terminal sanitising and width.

What changed

• src/core/text.ts (new): one strip, a small scanner over code points. It removes escape sequences (CSI; OSC/DCS/APC/PM/SOS strings up to BEL or ST; other ESC sequences; 7-bit and 8-bit forms) and control characters (C0 except tab/newline, DEL, C1, bidi embeddings/overrides/isolates, LRM/RLM/ALM, ZWSP, BOM, U+2028/2029). stripControlCharsDeep walks a lesson-shaped record: prose keys (summary, body) keep their line breaks, every other string is folded to one line.
• coach.importSharedLesson strips the whole payload before it compares or stores anything, then re-validates it with SharedLessonSchema. A lesson left without a title, topic or id is refused (invalid: 1). The legacy JSON-array form that import also accepts goes through the same strip.
• CLI print boundary: every lesson and course string goes through the strip again before it is printed (printable() in commands.ts: lessons, lesson, import, courses, course). The same goes for share's text output, topic names in profile/stats/feedback, and doctor's last skip reason, which the model writes. This covers rows stored before this fix.
• src/cli/term.ts:
  • displayWidth(): per grapheme via Intl.Segmenter. Emoji presentation and East Asian Wide/Fullwidth count 2, combining marks and zero-width characters 0. pad, renderTable and rule use it, so the 💡 row in lessons aligns.
  • sanitizeForTerminal() / sanitizeLine(): the CLI-facing names of the strip, for PR 7's renderer.
  • trustedLink() (was link()): wraps only a lowercase http(s):// URL with no whitespace, control or invisible format character (the same strip must leave it unchanged), at most 2 KiB. Anything else prints as plain text, and the label is sanitised too.
  • Colour: NO_COLOR wins. FORCE_COLOR forces colour (even on a pipe) unless it is 0/false. TERM=dumb disables colour unless forced.
  • termWidth(): explicit width, then the terminal's, then COLUMNS, then 80. readingWidth() caps it at 100.
  • rule() is now left-anchored: ── text ───….
• Docs: docs/usage/sharing.md (imported text is stored inert; the example table regenerated, it showed the misaligned row), docs/usage/cli.md (FORCE_COLOR, FORCE_HYPERLINK, COLUMNS), CLAUDE.md.

Decisions

• A scanner, not util.stripVTControlCharacters or a regex. Node's built-in leaves visible residue on DCS strings, 8-bit OSC and some ST-terminated OSC 8 links. A first version used one ECMA-48 regex; SonarCloud failed the reliability gate on it (S6324, control characters in a regex, the rule the maintainer marked won't fix on term.ts's ANSI regex). Its complexity was also 32 against the allowed 20. The scanner has the same behaviour (the same adversarial suite passes) and is linear: a test bounds 256 KiB of hostile input. Dropping every leftover control character is the actual guarantee: no ESC, C1 or BEL survives, so nothing a terminal could interpret survives. Removing whole sequences only keeps their parameters from showing up as text.
• ZWJ and ZWNJ are kept (v2 listed U+200B–U+200F). Stripping ZWJ splits emoji sequences (👨‍👩‍👧‍👦 → four faces), and ZWNJ is needed by Persian and Indic text. Neither reorders or hides anything. ZWSP, LRM, RLM and ALM are stripped.
• An unterminated control string stops at the end of its line, so a stray ESC ] cannot swallow the rest of a lesson.
• Every payload field is stripped on import, not only title/summary/body/shared_by: topic_id, categories, the context fields, and origin.id, which becomes the local lesson id.
• No migration of rows already stored. The CLI strips them at print time, the dashboard escapes HTML, and MCP returns them JSON-escaped. A data migration would rewrite user data for a case that only a crafted share produces.
• restore is not stripped: a backup is the user's own data, and the print-time strip covers the CLI anyway.
• Glyphs unchanged here. §5 gives the ✅ ✨ ❌ swap to PR 4 (vocabulary) and PR 7 (screens). This PR ships the width table that makes it safe; tests pin ✅ ✨ ❌ at 2 columns and ★ ✓ ✦ ✗ at 1.
• rule() width: the terminal width capped at 100, or 80 on a pipe without COLUMNS. Visible change: the lesson <id>, course <id> and setup headers go from a 60-column centred rule to a left-anchored one. The title stays inside the rule until PR 7 moves it out.
• FORCE_COLOR="" counts as forced (as in Node and supports-color). NO_COLOR="" counts as unset (no-color.org). styleText runs with validateStream: false, so Node's own check, which lets FORCE_COLOR beat NO_COLOR and prints a warning, never runs. The hook and MCP paths never use the styling helpers, so their JSON stays plain whatever FORCE_COLOR says.
• Ambiguous-width characters (─ │ █ ○ ★) count 1, as terminals do by default.
• Tables are still not fitted to the terminal width (deferred in v2 §4.3).

Tests

• tests/term.test.ts (rewritten, 81 cases):

  • sanitiser: CSI (clear, colour, private modes); OSC 8 to javascript:/file: with BEL and ST terminators; OSC 52 (BEL/ST, base64 with +/=); OSC 0; DCS/APC/PM/SOS; 8-bit CSI/OSC/DCS/APC with 8-bit ST; RIS and charset switches; bare C0/DEL/C1; RLO, isolates, LRM/RLM/ALM, ZWSP, BOM, line separators; unterminated strings. An exhaustive loop checks every C0/DEL/C1 code point alone and next to text, and text that must survive (newlines, tabs, ZWJ family, CJK, accents, markdown) is unchanged.
  • trustedLink with 10 hostile URLs. A timing bound on 256 KiB of hostile input. Colour decision table, and the decision reaching c.bold (vi.resetModules + vi.stubEnv).
  • displayWidth for emoji, ZWJ, flags, skin tones, keycaps, VS15/VS16, combining marks, CJK, Hangul, fullwidth.
  • renderTable with wide glyphs: every line has the same width and its borders in the same columns.
  • termWidth precedence, and rule() widths with wide text.

• tests/core.test.ts: importSharedLesson stores every field inert (no match for the v2 control regex). The same hostile share re-imported is a duplicate, an escapes-only title is refused, the legacy array is stripped. Plus stripControlCharsDeep on plain, null-prototype and non-plain values.

• tests/cli.test.ts:

  • a crafted share imported through devcoach import, then lessons, lesson and share: no control character in any output;
  • a raw row stored before this fix, through lessons, lesson, share, feedback know, profile and stats;
  • course text;
  • the lessons table aligning with 💡 and CJK titles.

  With the print-time strip disabled, the raw-row and course tests fail. With it on and the import strip off, the core test fails.

• npm run lint: clean (the 3 useTemplate infos in tests/cli.test.ts are pre-existing). npm run typecheck: clean. npm run test:cov: 470 tests pass. Coverage 94.97 lines / 93.77 statements / 96.93 functions / 84.72 branches (thresholds 92 / 91 / 95 / 76).85 statements / 97.03 functions / 84.72 branches (thresholds 92 / 91 / 95 / 76). term.ts and text.ts 100 % lines.

• The built CLI (npm run build, node dist/bin.js) against the crafted code on a sandbox writes 0 ESC and 0 BEL bytes for import, lessons and lesson, and the stored row is clean.

CHANGELOG

Suggested lines, not edited here: the request-guard PR adds the Security heading to Unreleased, so these go under it at merge time.

### Security
- The CLI no longer passes terminal escape sequences from a shared lesson to your terminal. Up to 2.7.1, an imported lesson could clear the screen, show a hidden link to a `javascript:` or `file:` URL, or write your clipboard (OSC 52) when `devcoach import`, `lessons` or `lesson` printed it. Imports now strip escape sequences and control characters, and the CLI strips them again before it prints any lesson.

### Fixed
- `devcoach lessons` columns line up with emoji and CJK titles (the 💡 row was one column off).
- `FORCE_COLOR=1` forces colour on a pipe and `FORCE_COLOR=0` turns it off; `TERM=dumb` turns colour off; `NO_COLOR` still wins.

Before / after

Slideshow — each screen BEFORE (red band: without this PR), then AFTER (green band: with this PR), with one line saying what changed. It plays by itself; click it to open it full size.

Dark

Videos — the first frame is the title; a full red BEFORE or green AFTER card opens every segment, and the band in the same colour stays above the recording until the segment ends.

Light mode, real CLI output rendered in xterm.js. BEFORE (develop, 2.7.1): in devcoach lessons the 💡 row's borders sit one column right of the header's. Importing a crafted shared lesson erases the screen and the scrollback, and devcoach lesson erases them again, then prints two hidden OSC 8 links (javascript:alert(1), file:///etc/passwd) and an OSC 52 clipboard write: 10 escape sequences in all. AFTER (this branch): the table lines up, and the same lesson prints as plain text; no escape sequence reaches the terminal.

Dark mode, same script. BEFORE (develop, 2.7.1): in devcoach lessons the 💡 row's borders sit one column right of the header's. Importing a crafted shared lesson erases the screen and the scrollback, and devcoach lesson erases them again, then prints two hidden OSC 8 links (javascript:alert(1), file:///etc/passwd) and an OSC 52 clipboard write: 10 escape sequences in all. AFTER (this branch): the table lines up, and the same lesson prints as plain text; no escape sequence reaches the terminal.

Side by side — BEFORE on the left, AFTER on the right.

devcoach lessons zoomed on the 💡 row, light. Before: from the end of the title on, every border sits one column right of the header's. After: the borders line up.

Dark

devcoach lesson on the imported lesson, light. Before: the screen was erased, “the BuildKit guide” is a live link to file:///etc/passwd, and the terminal received 10 escape sequences. After: the whole session is still on screen, the text is plain, and no sequence was received.

Dark

Not verified

• OSC 52 in a real terminal: the recording renders the real CLI output in xterm.js. Its parser hooks list the sequences the terminal received, and the clipboard write is shown as received, not performed. Whether a given terminal applies OSC 52 or opens a file: link depends on its settings.
• Windows terminals, and terminals or CJK locales that draw ambiguous-width characters as 2 columns.

🤖 Generated with Claude Code